-
-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: orbit details #3348
Fix: orbit details #3348
Conversation
- the intention is to start using this for storing detail settings without having to do the global store.
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
116f8d1
to
c6c3411
Compare
src/core/modules/SolarSystem.cpp
Outdated
if (!flagIsolatedOrbits) | ||
{ | ||
for (const auto& p : qAsConst(systemPlanets)) | ||
if (!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && (p->getPlanetType()==Planet::isPlanet || (p->parent && p->parent->getPlanetType()==Planet::isPlanet) ))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly &&...)
is redundant: the second check for the flag yields true
whenever it's evaluated. After you remove it, you can also get rid of two pairs of parentheses, yielding this simpler condition:
if (!flagPlanetsOrbitsOnly || p->getPlanetType()==Planet::isPlanet ||
(p->parent && p->parent->getPlanetType()==Planet::isPlanet))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed. This is currently written mostly documentarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern here is that it makes it harder to understand the condition, not only because of an extra check, but also due to multiple nested levels of parentheses.
src/core/modules/SolarSystem.cpp
Outdated
{ | ||
// Display only orbit for selected planet and its moons. | ||
for (const auto& p : qAsConst(systemPlanets)) | ||
if ( (p==selected && ( !flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && p->getPlanetType()==Planet::isPlanet ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same redundancy of the second check of flagPlanetsOrbitsOnly
here.
src/core/modules/SolarSystem.cpp
Outdated
for (const auto& p : qAsConst(systemPlanets)) | ||
if ( (p==selected && ( !flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && p->getPlanetType()==Planet::isPlanet ) ) | ||
|| (p->getPlanetType()==Planet::isMoon && p->parent==selected ) )) | ||
p->setFlagOrbits(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're basically passing the result of the condition check, you can change
if(cond) set(true); else set(false);
to set(cond);
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I just followed the existing style, but we can change that.
src/core/modules/SolarSystem.cpp
Outdated
// 1 1 1 0 only selected planet and orbits of its moon system | ||
// 1 1 1 1 only selected SSO if it is a major planet | ||
|
||
if (!flagOrbits || (flagOrbits&&flagIsolatedOrbits&&(!selected || selected==sun))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, same redundancy with flagOrbits
here as the ones below.
src/core/modules/SolarSystem.cpp
Outdated
{ | ||
for (const auto& p : qAsConst(systemPlanets)) | ||
if (!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && (p->getPlanetType()==Planet::isPlanet || (p->parent && p->parent->getPlanetType()==Planet::isPlanet) ))) | ||
p->setFlagOrbits(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here about passing a Boolean condition.
What about functionality? Shall moons for the planets be co-selected when "planet only" is active? Shall the single selected planet also show its moons' orbits? Or should we deal with the moons with yet another flag? Shall a selected object like the Moon (or other planet moons) show its orbit even if "only planet orbits" and "selected only" are active? |
What about a whole new setting planet + moon orbit? |
This is what I am asking: Show moons automatically (like now), or only if allowed by another switch? |
Oh yeah, sorry. A new flag, I would prefer. |
My wish is that what's written on the checkbox, that would be seen in the scene, without surprises. If the checkbox says only selected planet, then only the planet's orbit would be visible. So I guess this would mean a radiobutton: ⵙ selected planet |
I don’t like “immediate save” implementation - I think it should be implemented as new class/wrapper with static methods for saving the options + public method for saving all options. |
I dislike the Note that for now all these these settings are also still stored in ConfigDialog, so if immediate saving is not active, behaviour is unchanged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work as expected.
Hello @gzotti! Please check the fresh version (development snapshot) of Stellarium: |
Hello @gzotti! Please check the latest stable version of Stellarium: |
Description
As discussed in #2043, the sequence of selection around orbit details is inconsistent and buggy.
I separated flag setting from the actual selection (which involves 4 flags and an optional selection).
We can discuss yet another finetuning option "with moons" which would influence whether the orbital system of moons would be highlighted even if the flag for only "major planets" or "only selected object" is set and the selected object is a major planet.
Also, as first commit, I introduce a (currently hidden and manually editable only) option StelApp.flagImmediateSave. If set, this can be used to store settings immediately. The intention is to allow storing detail settings in the GUI, like orbit drawing details, DSO catalog selection or type filtering, without having to save these globally (and thereby maybe accidentally overwrite and globally store other settings that were active only during this session). My personal preference would be to get rid of the global save entirely, but I see resistance. Maybe we can offer a global switch on "immediate save" vs "global save", when all settings have been adjusted over the next few months.
I will amend the second commit by removing commented code and optionally adding the "with moons" option.
After this has been merged, PR #3342 will be adjusted accordingly.
Fixes #1842 (supersedes PR #2043)
Screenshots (if appropriate):
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: